Store the document PDF on quorum request - #1794
Conversation
709f7ac to
beae13d
Compare
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
beae13d to
77e6d8e
Compare
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1e284c0 to
0392e95
Compare
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
d919f37 to
c412cb0
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
c412cb0 to
dda2602
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
A refused quorum previously had no file of its own, so the reviewed version was lost after a later quorum was accepted. Generate the PDF in a dedicated worker so Chrome does not hold database locks, and keep the file internal to the quorum row. Signed-off-by: Sacha Al Himdani <sacha@probo.com>
dda2602 to
4c0bc92
Compare
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/probo/document_approval_service.go">
<violation number="1" location="pkg/probo/document_approval_service.go:1113">
P2: When the attach transaction fails or the PDF claim expires during generation, this upload leaves an unreferenced private object because the subsequent transaction rolls back and the worker retries. Delete the uploaded object on every post-upload failure, including the lost-claim path, or use a cleanup mechanism for these orphaned uploads.</violation>
</file>
<file name="pkg/probo/document_approval_quorum_pdf_worker.go">
<violation number="1" location="pkg/probo/document_approval_quorum_pdf_worker.go:65">
P1: When a quorum is rejected before this worker claims it, the worker can never process it because `ClaimNextWithoutFileForUpdate` only selects `PENDING` rows. Queue the PDF at quorum creation or allow the claim to cover the terminal quorum state so rapid refusals still retain their reviewed version.</violation>
</file>
<file name="pkg/coredata/document_version_approval_quorum.go">
<violation number="1" location="pkg/coredata/document_version_approval_quorum.go:490">
P1: When a quorum is rejected, approved, or voided before PDF processing completes, these `PENDING` checks make the worker abandon the claim, so the quorum remains fileless. Select unfiled quorums regardless of final status and validate and attach using the claim timestamp and `file_id IS NULL`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| if err := h.service.pg.WithTx( | ||
| ctx, | ||
| func(ctx context.Context, tx pg.Tx) error { | ||
| return quorum.ClaimNextWithoutFileForUpdate( |
There was a problem hiding this comment.
P1: When a quorum is rejected before this worker claims it, the worker can never process it because ClaimNextWithoutFileForUpdate only selects PENDING rows. Queue the PDF at quorum creation or allow the claim to cover the terminal quorum state so rapid refusals still retain their reviewed version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/probo/document_approval_quorum_pdf_worker.go, line 65:
<comment>When a quorum is rejected before this worker claims it, the worker can never process it because `ClaimNextWithoutFileForUpdate` only selects `PENDING` rows. Queue the PDF at quorum creation or allow the claim to cover the terminal quorum state so rapid refusals still retain their reviewed version.</comment>
<file context>
@@ -0,0 +1,122 @@
+ if err := h.service.pg.WithTx(
+ ctx,
+ func(ctx context.Context, tx pg.Tx) error {
+ return quorum.ClaimNextWithoutFileForUpdate(
+ ctx,
+ tx,
</file context>
| WHERE | ||
| q.file_id IS NULL | ||
| AND q.pdf_attempt_count < @max_pdf_attempts | ||
| AND q.status = @status |
There was a problem hiding this comment.
P1: When a quorum is rejected, approved, or voided before PDF processing completes, these PENDING checks make the worker abandon the claim, so the quorum remains fileless. Select unfiled quorums regardless of final status and validate and attach using the claim timestamp and file_id IS NULL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/coredata/document_version_approval_quorum.go, line 490:
<comment>When a quorum is rejected, approved, or voided before PDF processing completes, these `PENDING` checks make the worker abandon the claim, so the quorum remains fileless. Select unfiled quorums regardless of final status and validate and attach using the claim timestamp and `file_id IS NULL`.</comment>
<file context>
@@ -435,3 +459,228 @@ WHERE
+WHERE
+ q.file_id IS NULL
+ AND q.pdf_attempt_count < @max_pdf_attempts
+ AND q.status = @status
+ AND d.deleted_at IS NULL
+ AND (
</file context>
| UpdatedAt: now, | ||
| } | ||
|
|
||
| fileSize, err := s.svc.fileManager.PutFile( |
There was a problem hiding this comment.
P2: When the attach transaction fails or the PDF claim expires during generation, this upload leaves an unreferenced private object because the subsequent transaction rolls back and the worker retries. Delete the uploaded object on every post-upload failure, including the lost-claim path, or use a cleanup mechanism for these orphaned uploads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/probo/document_approval_service.go, line 1113:
<comment>When the attach transaction fails or the PDF claim expires during generation, this upload leaves an unreferenced private object because the subsequent transaction rolls back and the worker retries. Delete the uploaded object on every post-upload failure, including the lost-claim path, or use a cleanup mechanism for these orphaned uploads.</comment>
<file context>
@@ -1011,6 +1013,148 @@ func (s *DocumentApprovalService) generateApprovalPDF(
+ UpdatedAt: now,
+ }
+
+ fileSize, err := s.svc.fileManager.PutFile(
+ ctx,
+ fileRecord,
</file context>
A refused quorum previously had no file of its own, so the reviewed version was lost after a later quorum was accepted.
Generate the PDF in a dedicated worker so Chrome does not hold database locks, and keep the file internal to the quorum row.
Summary by cubic
A refused quorum previously had no file of its own, so the reviewed version was lost after a later quorum was accepted. This PR stores the reviewed document as a PDF on the quorum row, generated by a dedicated worker so Chrome never holds database locks, and exposes the quorum through the GraphQL Node interface.
Coredata
+289-13file_id,pdf_attempt_count, andpdf_claimed_atto quorums via a new migration.ClaimNextWithoutFileForUpdateatomically claims the oldest pending quorum without a PDF, skipping locked rows and expired claims, and increments the attempt counter.HasPDFClaim,AttachPDFFile, andReleasePDFClaimguard the claim lifecycle so the file attaches only while the claim is held and failures retry.Service
+341-11loadDocumentPDFInputandrenderDocumentPDFfor reuse, and wires the worker intoprobodstartup and shutdown.GraphQL API
+10-0ActionDocumentVersionApprovalListpermission.Written for commit 4c0bc92. Summary will update on new commits.